-
Notifications
You must be signed in to change notification settings - Fork 13.7k
Fix uplifting in Assemble
step
#145557
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix uplifting in Assemble
step
#145557
Conversation
Ok I realize another error now (which is already fixed in this PR). Before #145310, bootstrap used to say "uplifting stage1 rustc to stage3", but that's clearly bogus, it was actually uplifting the stage2 compiler. But I trusted the message and implemented uplifting of stage 1 in #145310, which is wrong (because of the ABI difference). |
This comment has been minimized.
This comment has been minimized.
my condolences on the extra space |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was not obvious 😭 Thanks, r=me after comment fix.
@@ -1441,31 +1462,51 @@ fn rustc_llvm_env(builder: &Builder<'_>, cargo: &mut Cargo, target: TargetSelect | |||
} | |||
} | |||
|
|||
/// `RustcLink` copies all of the rlibs from the rustc build into the previous stage's sysroot. | |||
/// `RustcLink` copies compiler rlibs from a rustc build into a compiler sysroot. | |||
/// It works with (potentially up to) three compilers: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remark: 😰
@rustbot author |
@bors r=jieyouxu |
Fix uplifting in `Assemble` step In rust-lang#145310, I removed [this line](https://github.com/rust-lang/rust/pull/145310/files#diff-5a1e05f2688d271039171a547d407d0c8a96715ee64d35562fc76b4c9a874303L2109), which adjusted the stage of the build compiler if an uplift has happened. This broke stage3+ uplifted rustc builds (rust-lang#145534). I could swear I tested this in the PR, but somehow I missed it. Instead of keeping the original returned stage, I made it more explicit by returning the actually used `build_compiler` from the `Rustc` step, and then use that in the `Assemble` step. The changes to `RustcLink` were needed to fix `ui-fulldeps`, which apparently build a stage3 rustc, because I haven't fixed the test steps yet 😅 Hopefully we might be able to remove `RustcLink` if the approach from rust-lang#144252 will work. Should fix rust-lang#145534. r? `@jieyouxu`
Rollup of 19 pull requests Successful merges: - #140956 (`impl PartialEq<{str,String}> for {Path,PathBuf}`) - #141744 (Stabilize `ip_from`) - #142681 (Remove the `#[no_sanitize]` attribute in favor of `#[sanitize(xyz = "on|off")]`) - #142871 (Trivial improve doc for transpose ) - #144252 (Do not copy .rmeta files into the sysroot of the build compiler during check of rustc/std) - #144476 (rustdoc-search: search backend with partitioned suffix tree) - #144567 (Fix RISC-V Test Failures in ./x test for Multiple Codegen Cases) - #144804 (Don't warn on never to any `as` casts as unreachable) - #144960 ([RTE-513] Ignore sleep_until test on SGX) - #145013 (overhaul `&mut` suggestions in borrowck errors) - #145041 (rework GAT borrowck limitation error) - #145243 (take attr style into account in diagnostics) - #145405 (cleanup: use run_in_tmpdir in run-make/rustdoc-scrape-examples-paths) - #145432 (cg_llvm: Small cleanups to `owned_target_machine`) - #145484 (Remove `LlvmArchiveBuilder` and supporting code/bindings) - #145557 (Fix uplifting in `Assemble` step) - #145563 (Remove the `From` derive macro from prelude) - #145565 (Improve context of bootstrap errors in CI) - #145584 (interpret: avoid forcing all integer newtypes into memory during clear_provenance) Failed merges: - #145359 (Fix bug where `rustdoc-js` tester would not pick the right `search.js` file if there is more than one) - #145573 (Add an experimental unsafe(force_target_feature) attribute.) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #145557 - Kobzol:rustc-link-fix, r=jieyouxu Fix uplifting in `Assemble` step In #145310, I removed [this line](https://github.com/rust-lang/rust/pull/145310/files#diff-5a1e05f2688d271039171a547d407d0c8a96715ee64d35562fc76b4c9a874303L2109), which adjusted the stage of the build compiler if an uplift has happened. This broke stage3+ uplifted rustc builds (#145534). I could swear I tested this in the PR, but somehow I missed it. Instead of keeping the original returned stage, I made it more explicit by returning the actually used `build_compiler` from the `Rustc` step, and then use that in the `Assemble` step. The changes to `RustcLink` were needed to fix `ui-fulldeps`, which apparently build a stage3 rustc, because I haven't fixed the test steps yet 😅 Hopefully we might be able to remove `RustcLink` if the approach from #144252 will work. Should fix #145534. r? ``@jieyouxu``
Fix rustc uplifting (take two) The rustc uplifting logic is really annoying.. #145557 was not enough to fix it. Consider #145534 (comment): in this situation, we do a stage3 build of a cross-compiled rustc (it happens because we run `x test --stage 2`, which mistakenly builds a stage3 rustc, but it doesn't matter what casuses it, what matters is that the stage3 build isn't working). Currently, a stage3 cross-compiled build of rustc works like this: 1) stage0 (host) -> stage1 (host) 2) stage1 (host) -> stage2 (host) 3) stage2 (host) -> stage3 (target) The problem is that in the uplifting logic, I assumed that we will have a stage2 (target) rustc available, which we can uplift. And that would indeed be an ideal solution. But currently, we will actually build a stage2 (*host*) rustc, and only then start the cross-compilation. So the uplifting is broken. I spend a couple of hours trying to fix this, and do the uplifting "from the other direction", so that already when we assemble a stage3 rustc, we notice that an uplift should happen, and we only build stage1 (host) rustc, which also helps avoid one needless rustc build. However, this was relatively complicated and would require larger changes that I was not confident landing at this time. So instead I decided to do a much simpler fix, and just disable rustc uplifting when cross-compiling. Since we currently do the `stage2 (host) -> stage3 (target)` step, it should not actually affect stage3 cross-compiled builds in any way (I hope..), and should only affect stage4+ builds, about which I don't really care (the only change there should be more rustc builds). For normal builds, the stage2 host rustc should (hopefully) always be present, so we shouldn't run into this issue. Eventually, I would like to remove rustc uplifting completely. However, `x test --stage 2` on CI still currently builds a stage3 rustc for some reason, and if we removed uplifting completely, even for non-cross-compiled builds, that would cause an additional rustc build, and that's not great. So for now let's just allow uplifting for non-cross-compiled builds. Fixes #145534. r? `@jieyouxu`
Fix rustc uplifting (take two) The rustc uplifting logic is really annoying.. rust-lang/rust#145557 was not enough to fix it. Consider rust-lang/rust#145534 (comment): in this situation, we do a stage3 build of a cross-compiled rustc (it happens because we run `x test --stage 2`, which mistakenly builds a stage3 rustc, but it doesn't matter what casuses it, what matters is that the stage3 build isn't working). Currently, a stage3 cross-compiled build of rustc works like this: 1) stage0 (host) -> stage1 (host) 2) stage1 (host) -> stage2 (host) 3) stage2 (host) -> stage3 (target) The problem is that in the uplifting logic, I assumed that we will have a stage2 (target) rustc available, which we can uplift. And that would indeed be an ideal solution. But currently, we will actually build a stage2 (*host*) rustc, and only then start the cross-compilation. So the uplifting is broken. I spend a couple of hours trying to fix this, and do the uplifting "from the other direction", so that already when we assemble a stage3 rustc, we notice that an uplift should happen, and we only build stage1 (host) rustc, which also helps avoid one needless rustc build. However, this was relatively complicated and would require larger changes that I was not confident landing at this time. So instead I decided to do a much simpler fix, and just disable rustc uplifting when cross-compiling. Since we currently do the `stage2 (host) -> stage3 (target)` step, it should not actually affect stage3 cross-compiled builds in any way (I hope..), and should only affect stage4+ builds, about which I don't really care (the only change there should be more rustc builds). For normal builds, the stage2 host rustc should (hopefully) always be present, so we shouldn't run into this issue. Eventually, I would like to remove rustc uplifting completely. However, `x test --stage 2` on CI still currently builds a stage3 rustc for some reason, and if we removed uplifting completely, even for non-cross-compiled builds, that would cause an additional rustc build, and that's not great. So for now let's just allow uplifting for non-cross-compiled builds. Fixes rust-lang/rust#145534. r? `@jieyouxu`
In #145310, I removed this line, which adjusted the stage of the build compiler if an uplift has happened. This broke stage3+ uplifted rustc builds (#145534). I could swear I tested this in the PR, but somehow I missed it.
Instead of keeping the original returned stage, I made it more explicit by returning the actually used
build_compiler
from theRustc
step, and then use that in theAssemble
step.The changes to
RustcLink
were needed to fixui-fulldeps
, which apparently build a stage3 rustc, because I haven't fixed the test steps yet 😅Hopefully we might be able to remove
RustcLink
if the approach from #144252 will work.Should fix #145534.
r? @jieyouxu